Add mTLS auth modal and validation#628
Open
marcschwaiger wants to merge 2 commits intonetbirdio:mainfrom
Open
Conversation
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds mTLS authentication support to the reverse-proxy UI and types: extends the ChangesmTLS reverse-proxy flow
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
7 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/modules/reverse-proxy/ReverseProxyModal.tsx (1)
342-349: 💤 Low valueThe
canReuseStoredMTLSCertguard is sound, but the implicit contract deserves a commentThe logic correctly handles the "cert stored server-side, not echoed back" case: submitting
{ enabled: true, ca_cert_pem: undefined }tells the backend to preserve the stored cert. This is the right design, but a brief inline comment would help future maintainers understand whymtlsErroris suppressed when the PEM string is empty.💡 Suggested inline comment
+ // If the proxy was already configured with mTLS, the backend holds the cert and + // won't return it — submitting ca_cert_pem: undefined preserves the stored cert. const canReuseStoredMTLSCert = reverseProxy?.auth?.mtls_auth?.enabled === true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/reverse-proxy/ReverseProxyModal.tsx` around lines 342 - 349, Add a short inline comment above the canReuseStoredMTLSCert / mtlsError logic explaining the implicit contract: when reverseProxy?.auth?.mtls_auth?.enabled is true and the server has a stored cert, submitting ca_cert_pem as an empty string (mtlsCACertPEM.trim().length === 0) signals “preserve server-side cert,” so mtlsError should be suppressed; reference the canReuseStoredMTLSCert, mtlsEnabled, mtlsCACertPEM, and mtlsError symbols to clarify why an empty PEM is allowed and not treated as a validation failure.src/modules/reverse-proxy/auth/AuthMTLSModal.tsx (1)
112-121: ⚡ Quick winNo error handler on
FileReader— silent failure on read errorIf
readAsTextfails (e.g., filesystem permission denied, corrupted file), the user gets no feedback.🛠 Suggested fix
fileReader.readAsText(file, "UTF-8"); fileReader.onload = (e) => { if (e.target === null) return; handleFileText(e.target.result as string); }; + fileReader.onerror = () => { + // surface a user-visible error if needed, or at minimum log + console.error("Failed to read certificate file"); + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/modules/reverse-proxy/auth/AuthMTLSModal.tsx` around lines 112 - 121, handleFileUpload currently uses FileReader.readAsText without any error handling so read failures are silent; add a FileReader.onerror (and optionally onabort/onloadend) handler that captures the FileReader.error and surfaces it (e.g., call an existing error handler or set an error state) and ensure handleFileText is only called on successful load inside onload; update the FileReader usage in handleFileUpload to attach fileReader.onerror and propagate a clear error message referencing FileReader.error and the file name so the UI can show feedback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/modules/reverse-proxy/auth/AuthMTLSModal.tsx`:
- Around line 159-187: The upload container in AuthMTLSModal is not
keyboard-accessible; update the clickable div to be focusable and operable by
keyboard by adding tabIndex={0}, role="button", and an onKeyDown handler that
triggers inputRef.current?.click() when Enter or Space is pressed, and include
an accessible label (aria-label or aria-labelledby) describing the action; also
ensure the hidden file input (referenced by inputRef and used by
handleFileUpload) is not removed from interaction (replace the "hidden"
display:none class with a visually-hidden/sr-only technique or ensure the div's
keyboard handler reliably opens the file dialog) so keyboard-only users can open
the file picker.
- Around line 48-53: The leftover check in AuthMTLSModal rejects valid annotated
PEM bundles because comment/header lines (e.g., lines starting with "#" or other
non-PEM annotation lines) remain in trimmed before building leftover; preprocess
the input before the existing certificate-strip step by removing or filtering
out annotation lines (e.g., split trimmed into lines and drop lines that start
with "#" or that don't start with "-----") then rejoin to form trimmed for the
existing replace logic, and keep the subsequent leftover length check as-is
(adjust the error message in the same block if you prefer to mention unsupported
comment lines); refer to the variables/functions AuthMTLSModal, trimmed,
leftover in your changes.
---
Nitpick comments:
In `@src/modules/reverse-proxy/auth/AuthMTLSModal.tsx`:
- Around line 112-121: handleFileUpload currently uses FileReader.readAsText
without any error handling so read failures are silent; add a FileReader.onerror
(and optionally onabort/onloadend) handler that captures the FileReader.error
and surfaces it (e.g., call an existing error handler or set an error state) and
ensure handleFileText is only called on successful load inside onload; update
the FileReader usage in handleFileUpload to attach fileReader.onerror and
propagate a clear error message referencing FileReader.error and the file name
so the UI can show feedback.
In `@src/modules/reverse-proxy/ReverseProxyModal.tsx`:
- Around line 342-349: Add a short inline comment above the
canReuseStoredMTLSCert / mtlsError logic explaining the implicit contract: when
reverseProxy?.auth?.mtls_auth?.enabled is true and the server has a stored cert,
submitting ca_cert_pem as an empty string (mtlsCACertPEM.trim().length === 0)
signals “preserve server-side cert,” so mtlsError should be suppressed;
reference the canReuseStoredMTLSCert, mtlsEnabled, mtlsCACertPEM, and mtlsError
symbols to clarify why an empty PEM is allowed and not treated as a validation
failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3f4bd30a-2c97-42c5-bf48-56dabd662b75
📒 Files selected for processing (5)
src/interfaces/ReverseProxy.tssrc/modules/reverse-proxy/ReverseProxyModal.tsxsrc/modules/reverse-proxy/auth/AuthMTLSModal.tsxsrc/modules/reverse-proxy/events/ReverseProxyEventsAuthMethodCell.tsxsrc/modules/reverse-proxy/table/ReverseProxyAuthCell.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue ticket number and link
netbirdio/netbird#5364 mTLS Auth for Proxy Services
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
netbirdio/docs#720
See also: Netbird PR URL
netbirdio/netbird#6048
Summary by CodeRabbit